-
Notifications
You must be signed in to change notification settings - Fork 714
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use SHA-2 hash over MD5 #1320
Use SHA-2 hash over MD5 #1320
Conversation
59aad25
to
801e083
Compare
801e083
to
b34e0a7
Compare
Just an update. So I was able to get my hands on a FIPS enabled RHEL9 box. After building on the main branch, I get the same error of "Can't allocate memory" given the issue with MD5. I have tested with this PR's changes and was able to get a different error which was shown in the pipeline: "Can't verify database integrity". Upon closer inspection it seems like the CVD use MD5, so SHA256 will not work as expected. I believe that the error for failed memory allocation happens while the system tries to obtain the MD5 checksum (FIPS restricts this). If that is the case, then it seems that changes in this branch are successfully reading the SHA256 but can't validate it with the current CVD internal (as it's MD5). What I have currently tried out is by modifying If so, I'd like to test a SHA256 CVD, but I am not quite sure where to begin with this. Changing the checksum to be SHA256 inside the CVD does not work, so any advice on where I can find the relevant documentation would be greatly appreciated. I was hoping to perhaps redo the TLDR: SHA256 changes in branch seem to read, but fail because current CVD are all MD5 |
Your changes make sense, if we want to simply replace MD5 in the existing signing process with SHA256. But there is a lot more work that would need to be done. Our internal CVD signing code has the logic to be able to switch to SHA256 so that's also doable. The problem is that we'll need to continue to distribute the MD5 signed CVD's for older ClamAV versions, and also the newer SHA256 signed CVD's (new name?), and we'll have to put in additional changes in the readdb.c code (I think?) and freshclam code to differentiate. We'd then have to change our SigManager code to build and publish both versions. I think our web team will also need to change some stuff in cloudflare to host both and enable the same rate limiting rules, etc. If we're going through all that, then I really want to switch to a better signing mechanism that is more standard / less custom, and also allows us to rotate signing keys, etc. Stuff I started brainstorming here: #564 (comment) |
Hi, thanks for the update on the changes for this PR. I agree that distributing the old MD5 is still necessary... And moving everything to SHA256 is not the complete solution as users with older clamav will fail to work. Looking at your comment in I am wondering if it is possible to include both SHA256 and MD5 for a CVD, to facilitate the burden of having two separate builds? I'm not sure how logical it is for clamav to see if FIPS is enabled, then to use SHA256 - if not, default to MD5?
I'm personally in agreement that having a better signing mechanism can enhance security, and not a waste of time. I can try to pursue a solution for this, but I have limiting knowledge on this and will need some support.
I think that it should be fine to store the public keys for verifying the CVD2's (If we plan to use this new format) in the same directory. I'm curious to know if there are security concerns or edge cases where this might not be the best idea. If this helps unblock users with FIPS requirements, then please let me know if this seems fine to proceed. I imagine that the rotating keys can be in a separate PR? |
There's no reason to prefer MD5 over SHA256. The only reason to continue publishing with MD5's is for backwards compatibility with older clamav versions. If we sign the CVD's with MD5 and also with SHA256 while retaining compatibility with older CVD loading code, this would be a big win. Sadly, I also cannot share the CVD signing software for you to work on. I am not allowed to open source it. And I don't think it is reasonable to ask you to solve it without being able to do test signing and test changes to the signing process yourself. For what it's worth, I also want to resolve this issue. We have competing priorities and I haven't been able to dedicate dev time to it, yet. |
Yes, I agree with this as it might be the best approach for compatibility and lessens the maintenance required.
Which is perfectly reasonable. Thank you for the clarification on this.
Would it be fine for me to leave this PR open until the CVD's have been updated? I can retest this branch again once the new files are promoted. |
It doesn't bother me. Leaving it open is a visual reminder, and an indication that I do consider this work to be important. |
I would like to keep this draft PR open to leverage the community for advice / guidance on supporting FIPS-140 through deprecating MD5 in favor of SHA-2. I think that it would be best to work towards the short term solution to unblock users with FIPS-140 requirements safely, while longer term solutions are in the works.
In reference to: #564